[GUI] Fix ActivateWindow if 'return' is defined #17893

enen92 · 2020-05-19T15:17:00Z

Description

This pull request fixes the usage of ActivateWindow(window, dir, return) if the action is executed from context menu items (which according to the bug report is there since krypton).

This is mainly the case in python addons where listitems are created (for example) with the ActivateWindow(xxxx, plugin://plugin.video.foo/bar, return) as context menu actions. Other examples are any other dir paths to MusicLibrary and VideoLibrary.

According to the documentation, it is expected that Kodi moves back to the plugin directory from which the action was executed. However this doesn't work because:

Kodi always computes the history for the provided path even if we specify the return parameter (e.g. we provide "plugin://plugin.video.foo/bar" as dir, Kodi will create the respective history: plugin://plugin.video.foo/bar ../ plugin://plugin.video.foo)
Kodi always defines the provided dir as the root folder. This works well if we are activating a different window than the one that is already active. If they are the same, the root folder of dir should be set to the path that was on the media window before activating this new one otherwise we won't be able to move back to where we were.

@mediaminister @dagwieers since you were the ones reporting the issue, it'd be nice if you guys could test it.

I am unsure who I should ping for review, this seems no-man's code by now.

Motivation and Context

Fixes #16492

How Has This Been Tested?

Context menu item created in a plugin that pointed to another plugin:

    myitem = xbmcgui.ListItem("TEST")
    myitem.addContextMenuItems([("move to programs", "ActivateWindow(Videos,plugin://plugin.video.vrt.nu/programs/emma/allseasons, return)")])
    xbmcplugin.addDirectoryItem(handle=plugin.handle, listitem=myitem, isFolder=True, url=plugin.url_for(live))

Confirmed the onBack action moved back to where I was before. Also tested library navigation (movies and tvshows) in estuary and confirmed the behaviour is still the same.

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

DaveTBlake · 2020-05-19T15:44:36Z

Take care you don't break other behaviour with this @enen92 , or ways in which skinners have become used to using this and Kodi behaving. I am unable to test immediately, and @the-black-eagle may be interested

enen92 · 2020-05-19T15:52:25Z

@DaveTBlake I was about to ping you. Unfortunately we never know if regressions are introduced. This needs extensive testing and won't be merged unless it's proven okay. I'll ask in #skinning and maybe #testing

DaveTBlake · 2020-05-19T16:00:04Z

Sure @enen92 I wasn't saying don't, just aware from looking at other parts of the window/history/navigation behavour that it can bite back.

mediaminister · 2020-05-20T06:14:32Z

Because of this bug the VRT NU add-on moved to using Container.Update(plugin_url) which also works.
To test this I changed it back to ActivateWindow(Videos,plugin_url,return) and I can confirm this works now as it should be.

Thanks a lot for fixing this bug.

enen92 · 2020-05-21T21:59:22Z

One "regression" found by @sualfred (or to be more precise something that didn't correspond to his expected behaviour - although still better than we have now):

Test case:
HOME -> TvShows -> Select a TVShow -> Select a random season -> JSON RPC: { "jsonrpc": "2.0", "method":"GUI.Activatewindow","params":{"window":"videos","parameters":["videodb://movies","return"]},"id":"1"} -> (moves to movies as expected)

Navigating back:

PR (old implementation): Back -> episode view -> back -> home
Master: Back -> HOME
Expected Behaviour: Back -> episode view -> (back) seasons -> back (shows) -> back (HOME)

I adjusted the code to comply to the expected behaviour.

Would still be nice to have some feedback on the music section.

DaveTBlake · 2020-05-22T07:01:40Z

Would still be nice to have some feedback on the music section

Understood @enen92 but been a bit busy with release and sunny summer weather. :)
Will take a look soonest.

the-black-eagle · 2020-05-22T07:09:12Z

Tested in the music section by opening various nodes with both musicdb:// and library://music/ type paths. Didn't see any regressions and with return defined always returned to the nav window that the node was activated from. Without return defined, some nodes return to the root node list when navigating before returning to the window they were called from.
LGTM but @DaveTBlake may spot something I missed :)

DaveTBlake · 2020-05-22T07:16:26Z

Did you check the recently adjusted version @the-black-eagle ?

enen92 · 2020-05-22T09:51:44Z

Thanks @the-black-eagle (hopefully you cherry-picked today since I changed the implementation last night). @DaveTBlake no rush at all, was just a reminder I didn't test the MusicLibrary :).

This needs yet to be validated with ReplaceWindow yet as it shares the same implementation as ActivateWindow and I'm pretty sure the behaviour will not be 100% identical after my changes.

Rechi

Please follow the current code guidelines.

xbmc/windows/GUIMediaWindow.cpp

enen92 · 2020-05-22T12:04:53Z

Tested with ReplaceWindow() and it behaves correctly too:

Test case1:
Home -> RunAddon -> "ReplaceWindow(videos,videodb://movies/titles, return)" on the addon entrypoint -> moves to movie titles

Test case2:
Home -> RunAddon -> "ReplaceWindow(videos,videodb://movies/titles)" on the addon entrypoint -> moves to movie titles

OnBack navigation for test case 1:
Back -> home

OnBack navigation for test case 2:
Back -> videodb://movies -> Back -> videos -> Back -> Home

Weather in estuary (where replace window is used) also behaves identical.

enen92 · 2020-05-24T09:12:50Z

jenkins build this please

DaveTBlake

Sorry for the weeks of delay @enen92, tested out the music library navigation and all looks fine to me

enen92 · 2020-06-12T14:57:10Z

Thanks a lot @DaveTBlake for testing and no worries about the time taken!

[GUI] Fix ActivateWindow if 'return' is defined

Nanomani · 2020-06-15T07:36:29Z

No Backport ?

dagwieers · 2020-06-18T13:07:28Z

@enen92 Can we backport this to Leia?

enen92 · 2020-06-18T13:29:11Z

Although I didn't spot any regressions I'd prefer not to. It's better be prepared to find any regression in v19 development cycle than to introduce regressions on a version that is supposed to (TM) be the last one (leia). But you are always welcome to bring it as a pull request and see what others have to say about it. Personally I think it's time to move on and focus 100% on matrix

dagwieers · 2020-06-18T13:47:58Z

I understand, but we add-on developers will have to work with Leia for a few years, and it helps if we can reduce the number of workarounds in our code.

(Also when we proposed a fix for v18.3 someone used this argument because it was supposed to be the final update. Things do not always go as planned 😉)

[GUI] Fix ActivateWindow if 'return' is defined

enen92 added Type: Fix non-breaking change which fixes an issue Component: GUI engine v19 Matrix labels May 19, 2020

enen92 added this to the Matrix 19.0-alpha 1 milestone May 19, 2020

dagwieers mentioned this pull request May 21, 2020

Returning from "Go to program" fails, ends up in Videos add-ons/plugin.video.vrt.nu#435

Open

enen92 force-pushed the fixactivate branch from 035817b to 7d55002 Compare May 21, 2020 21:45

Rechi reviewed May 22, 2020

View reviewed changes

xbmc/windows/GUIMediaWindow.cpp Outdated Show resolved Hide resolved

[GUI] Fix ActivateWindow if 'return' is defined

bb5841b

enen92 force-pushed the fixactivate branch from 7d55002 to bb5841b Compare May 22, 2020 10:55

DaveTBlake approved these changes Jun 12, 2020

View reviewed changes

enen92 merged commit 9a8ae9f into xbmc:master Jun 12, 2020

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 14, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

fd53fdc

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 15, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

6fb8eea

[GUI] Fix ActivateWindow if 'return' is defined

dagwieers mentioned this pull request Jun 18, 2020

Fix ActivateWindow if 'return' is defined (Leia backport) #18073

Merged

13 tasks

DaveTBlake added the Backport: Done label Jul 15, 2020

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

7a02b19

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

e9f653c

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

c3f4a1c

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

416a187

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

61bdef4

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

b5d3010

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

9fd1eec

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

af08f49

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

04b0108

[GUI] Fix ActivateWindow if 'return' is defined

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020

Merge pull request xbmc#17893 from enen92/fixactivate

cbd6fdc

[GUI] Fix ActivateWindow if 'return' is defined

enen92 mentioned this pull request Aug 19, 2020

[Builtins][MediaWindow] Propagate window replace intention and set th… #18303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GUI] Fix ActivateWindow if 'return' is defined #17893

[GUI] Fix ActivateWindow if 'return' is defined #17893

enen92 commented May 19, 2020 •

edited

DaveTBlake commented May 19, 2020

enen92 commented May 19, 2020

DaveTBlake commented May 19, 2020

mediaminister commented May 20, 2020

enen92 commented May 21, 2020 •

edited

DaveTBlake commented May 22, 2020

the-black-eagle commented May 22, 2020

DaveTBlake commented May 22, 2020

enen92 commented May 22, 2020

Rechi left a comment

enen92 commented May 22, 2020

enen92 commented May 24, 2020

DaveTBlake left a comment

enen92 commented Jun 12, 2020

Nanomani commented Jun 15, 2020

dagwieers commented Jun 18, 2020

enen92 commented Jun 18, 2020 •

edited

dagwieers commented Jun 18, 2020 •

edited

[GUI] Fix ActivateWindow if 'return' is defined #17893

[GUI] Fix ActivateWindow if 'return' is defined #17893

Conversation

enen92 commented May 19, 2020 • edited

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

DaveTBlake commented May 19, 2020

enen92 commented May 19, 2020

DaveTBlake commented May 19, 2020

mediaminister commented May 20, 2020

enen92 commented May 21, 2020 • edited

DaveTBlake commented May 22, 2020

the-black-eagle commented May 22, 2020

DaveTBlake commented May 22, 2020

enen92 commented May 22, 2020

Rechi left a comment

Choose a reason for hiding this comment

enen92 commented May 22, 2020

enen92 commented May 24, 2020

DaveTBlake left a comment

Choose a reason for hiding this comment

enen92 commented Jun 12, 2020

Nanomani commented Jun 15, 2020

dagwieers commented Jun 18, 2020

enen92 commented Jun 18, 2020 • edited

dagwieers commented Jun 18, 2020 • edited

enen92 commented May 19, 2020 •

edited

enen92 commented May 21, 2020 •

edited

enen92 commented Jun 18, 2020 •

edited

dagwieers commented Jun 18, 2020 •

edited